feat(storage): Splitting table change log from HummockVersion on CN side #20050

Li0k · 2025-01-07T07:48:53Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

This PR optimize clone behavior on the CN side. Previously, the hummock event handler copied the hummock version every time it applied a delta, and the overhead of cloning could cause performance problems.issues.

risingwave/src/storage/src/hummock/event_handler/hummock_event_handler.rs

Line 529 in 95abcc9

let mut version_to_apply = pinned_version.version().clone();

This PR split the table change log from the hummock version to avoid copying all table change logs at each version delta.

Key changes include:

Enhancements to `HummockVersion` and `HummockVersionDelta`:

Introduced a second type parameter L to HummockVersionCommon and HummockVersionDeltaCommon to improve type safety and flexibility. Split table_change_log into separate fields and protect them with RwLock. (src/storage/hummock_sdk/src/version.rs, src/storage/hummock_sdk/src/compaction_group/hummock_version_ext.rs)

New Methods and Type Changes:

Added change_log_into_iter method to TableChangeLogCommon to allow iteration over change logs. (src/storage/hummock_sdk/src/change_log.rs)

Type Aliases:

Updated type aliases to use the new two-parameter versions of HummockVersionCommon and HummockVersionDeltaCommon. (src/storage/hummock_sdk/src/time_travel.rs, src/storage/hummock_sdk/src/version.rs)

Import Adjustments:

Adjusted imports to accommodate the new types and methods. (src/storage/hummock_sdk/src/compaction_group/hummock_version_ext.rs, src/storage/hummock_sdk/src/version.rs, src/storage/src/hummock/event_handler/hummock_event_handler.rs)

Checklist

I have written necessary rustdoc comments.
I have added necessary unit tests and integration tests.
I have added test labels as necessary.
I have added fuzzing tests or opened an issue to track them.
My PR contains breaking changes.
My PR changes performance-critical code, so I will run (micro) benchmarks and present the results.
My PR contains critical fixes that are necessary to be merged into the latest release.

Documentation

My PR needs documentation updates.

Release note

…nto li0k/storage_divide_table_change_log

zwang28 · 2025-01-08T06:17:20Z

src/storage/src/hummock/local_version/pinned_version.rs

            guard: Arc::new(PinnedVersionGuard::new(
                version_id,
                self.guard.pinned_version_manager_tx.clone(),
            )),
+            table_change_log: Arc::new(RwLock::new(t)),
+            version: Arc::new(LocalHummockVersion::from(version)),


I just want to leave a note here.

This LocalHummockVersion::from is an addtional HummockVersion conversion introduced by this PR. However I don't think it will have significant performance implications, as it primarily involves move semantics.

zwang28 · 2025-01-08T06:23:43Z

src/storage/src/hummock/store/version.rs

+        let change_log = {
+            let table_change_logs = version.table_change_log().read();
+            if let Some(change_log) = table_change_logs.get(&options.table_id) {
+                change_log.filter_epoch(epoch_range).cloned().collect_vec()


This cloned() is an additional cost introduced in this PR.

If multiple iter_log are running simultaneously, will the memory usage be substantial?

cc @wenym1 , suggests that iter_log is executed less frequently and that this clone is acceptable.

In current usage of iter_log, we only do iter_log on a single epoch, so this vector should be small.

zwang28

LGTM

…nto li0k/storage_divide_table_change_log

Li0k · 2025-01-13T09:08:59Z

interfaces are affected:

iter_log
~~- next_epoch~~

cc @wenym1

…nto li0k/storage_divide_table_change_log

wenym1

Rest LGTM. Thanks for the PR.

src/storage/hummock_sdk/src/version.rs

wenym1 · 2025-01-16T08:40:15Z

src/storage/src/hummock/store/version.rs

+        let change_log = {
+            let table_change_logs = version.table_change_log().read();
+            if let Some(change_log) = table_change_logs.get(&options.table_id) {
+                change_log.filter_epoch(epoch_range).cloned().collect_vec()


In current usage of iter_log, we only do iter_log on a single epoch, so this vector should be small.

src/storage/hummock_sdk/src/change_log.rs

src/storage/hummock_sdk/src/time_travel.rs

src/storage/src/hummock/local_version/pinned_version.rs

wenym1 · 2025-01-16T09:08:18Z

src/storage/src/hummock/event_handler/hummock_event_handler.rs

-                                .build_sst_delta_infos(version_delta)
-                                .into_iter(),
+                let mut version_to_apply = pinned_version.version().clone();
+                let table_change_log_to_apply = pinned_version.take_change_log();


Instead of take_change_log, we can have method change_log_write_lock to return the write lock on the table_change_log. And then in new_pin_version_with_table_change_log, we don't have to pass the table_change_log_to_apply again. We can use the original table_change_log wrapped by Arc and simply clone the Arc.

Besides, it seems that the parameter pinned_version of the current method is not necessary to take the ownership. We can change to pass &PinnedVersion, so that the caller can avoid doing clone on it.

…nto li0k/storage_divide_table_change_log

wenym1 · 2025-01-17T17:14:04Z

src/storage/hummock_sdk/src/version.rs

@@ -476,10 +473,26 @@ impl HummockVersion {
            state_table_info_delta: Default::default(),
        }
    }
+
+    pub fn split_change_log(mut self) -> (LocalHummockVersion, HashMap<TableId, TableChangeLog>) {
+        let table_change_log = std::mem::take(&mut self.table_change_log);


Here after std::mem::take(&mut self.table_change_log), the epochs in self.table_change_log becomes empty. But we still need to use the epochs in LocalHummockVersion

Good catch !!!

src/storage/hummock_sdk/src/version.rs

src/storage/src/hummock/local_version/pinned_version.rs

src/storage/src/hummock/event_handler/hummock_event_handler.rs

…nto li0k/storage_divide_table_change_log

gru-agent · 2025-01-20T06:06:28Z

This pull request has been modified. If you want me to regenerate unit test for any of the files related, please find the file in "Files Changed" tab and add a comment @gru-agent. (The github "Comment on this file" feature is in the upper right corner of each file in "Files Changed" tab.)

Li0k · 2025-01-20T06:06:43Z

@wenym1 PTAL

wenym1

Rest LGTM!

wenym1 · 2025-01-20T06:23:59Z

src/storage/hummock_sdk/src/version.rs

+                let mut change_log = VecDeque::new();
+                for item in log.change_log_iter_mut() {
+                    let new_value = EpochNewChangeLogCommon {
+                        new_value: std::mem::take(&mut item.new_value),
+                        old_value: std::mem::take(&mut item.old_value),
+                        epochs: item.epochs.clone(),
+                    };
+
+                    change_log.push_back(new_value);
+                }
+                table_change_log
+                    .insert(*table_id, TableChangeLogCommon::new(change_log.into_iter()));


Suggested change

let mut change_log = VecDeque::new();

for item in log.change_log_iter_mut() {

let new_value = EpochNewChangeLogCommon {

new_value: std::mem::take(&mut item.new_value),

old_value: std::mem::take(&mut item.old_value),

epochs: item.epochs.clone(),

};

change_log.push_back(new_value);

}

table_change_log

.insert(*table_id, TableChangeLogCommon::new(change_log.into_iter()));

let change_log_iter = log.change_log_iter_mut().map(|item| {

EpochNewChangeLogCommon {

new_value: std::mem::take(&mut item.new_value),

old_value: std::mem::take(&mut item.old_value),

epochs: item.epochs.clone(),

}

}

table_change_log

.insert(*table_id, TableChangeLogCommon::new(change_log_iter));

…nto li0k/storage_divide_table_change_log

Li0k added 2 commits January 7, 2025 15:45

feat(storage): Splitting table change log from HummockVersion on CN side

f9d8c93

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

73d6c39

…nto li0k/storage_divide_table_change_log

github-actions bot added the type/feature label Jan 7, 2025

Li0k changed the title ~~feat(storage): Splitting table change log from HummockVersion on CN side~~ WIP: feat(storage): Splitting table change log from HummockVersion on CN side Jan 7, 2025

github-actions bot added the Invalid PR Title label Jan 7, 2025

Li0k added 2 commits January 7, 2025 16:44

bug fix

dd7a205

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

3d8e652

…nto li0k/storage_divide_table_change_log

Li0k changed the title ~~WIP: feat(storage): Splitting table change log from HummockVersion on CN side~~ feat(storage): Splitting table change log from HummockVersion on CN side Jan 7, 2025

Li0k requested review from wenym1, hzxa21 and zwang28 and removed request for wenym1 and hzxa21 January 7, 2025 08:45

github-actions bot added Invalid PR Title and removed Invalid PR Title labels Jan 7, 2025

Li0k requested a review from hzxa21 January 7, 2025 08:45

Li0k removed the Invalid PR Title label Jan 8, 2025

zwang28 reviewed Jan 8, 2025

View reviewed changes

zwang28 approved these changes Jan 13, 2025

View reviewed changes

Li0k added 2 commits January 13, 2025 17:07

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

74fee69

…nto li0k/storage_divide_table_change_log

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

6b1e9a9

…nto li0k/storage_divide_table_change_log

Li0k added 3 commits January 13, 2025 18:19

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

95abcc9

…nto li0k/storage_divide_table_change_log

typo

d7ff917

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

31ba3c3

…nto li0k/storage_divide_table_change_log

wenym1 reviewed Jan 16, 2025

View reviewed changes

Li0k added 2 commits January 17, 2025 15:38

address comments

453ab6d

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

4acf1fd

…nto li0k/storage_divide_table_change_log

Li0k added this pull request to the merge queue Jan 17, 2025

typo

b91fc40

Li0k removed this pull request from the merge queue due to a manual request Jan 17, 2025

Li0k enabled auto-merge January 17, 2025 07:51

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

a704796

…nto li0k/storage_divide_table_change_log

Li0k added this pull request to the merge queue Jan 17, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 17, 2025

wenym1 added the ci/run-backfill-tests label Jan 17, 2025

wenym1 reviewed Jan 17, 2025

View reviewed changes

Li0k added 2 commits January 20, 2025 14:06

address comments

d516e38

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

0b84fc7

…nto li0k/storage_divide_table_change_log

wenym1 approved these changes Jan 20, 2025

View reviewed changes

Li0k added 2 commits January 20, 2025 15:59

address comments

b3f7f7b

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

afda1d0

…nto li0k/storage_divide_table_change_log

Li0k enabled auto-merge January 20, 2025 07:59

Li0k added this pull request to the merge queue Jan 20, 2025

Merged via the queue into main with commit bf8f076 Jan 20, 2025
29 of 30 checks passed

Li0k deleted the li0k/storage_divide_table_change_log branch January 20, 2025 09:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(storage): Splitting table change log from HummockVersion on CN side #20050

feat(storage): Splitting table change log from HummockVersion on CN side #20050

Li0k commented Jan 7, 2025 •

edited

Loading

zwang28 Jan 8, 2025

zwang28 Jan 8, 2025 •

edited

Loading

Li0k Jan 8, 2025

wenym1 Jan 16, 2025

zwang28 left a comment

Li0k commented Jan 13, 2025 •

edited

Loading

wenym1 left a comment

wenym1 Jan 16, 2025

wenym1 Jan 16, 2025

wenym1 Jan 17, 2025

Li0k Jan 20, 2025

gru-agent bot commented Jan 20, 2025

Li0k commented Jan 20, 2025

wenym1 left a comment

wenym1 Jan 20, 2025

feat(storage): Splitting table change log from HummockVersion on CN side #20050

feat(storage): Splitting table change log from HummockVersion on CN side #20050

Conversation

Li0k commented Jan 7, 2025 • edited Loading

What's changed and what's your intention?

Enhancements to HummockVersion and HummockVersionDelta:

New Methods and Type Changes:

Type Aliases:

Import Adjustments:

Checklist

Documentation

Choose a reason for hiding this comment

zwang28 Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zwang28 left a comment

Choose a reason for hiding this comment

Li0k commented Jan 13, 2025 • edited Loading

wenym1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gru-agent bot commented Jan 20, 2025

Li0k commented Jan 20, 2025

wenym1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Li0k commented Jan 7, 2025 •

edited

Loading

Enhancements to `HummockVersion` and `HummockVersionDelta`:

zwang28 Jan 8, 2025 •

edited

Loading

Li0k commented Jan 13, 2025 •

edited

Loading